Optimize unique tags #761
Open
Optimize unique tags #761
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We have two general code paths: 1. Merge two Tags in to one 2. Merge two Tags in to one, excluding certain tags We used to handle number 1 by passing an empty map to number 2, however these can be seen as fast (add static tags) and slow (add static tags + perform filtering) code-paths. The fast code path is now optimized by performing an array search instead of using a map to track things that have been seen. This is more efficient for small values. There is diminishing returns eventually, but that's with a high number of tags, which is an anti-pattern. The primary benefit is in removing the map allocation. The slow code path is unchanged, but could be improved by ensuring the map is sufficiently large, or using the same linear search method to check if a tag should be excluded. Note: this benchmark isn't perfect because we clone the source arrays, but that can be identified in the profile. BenchmarkUniqueTagsPractical/original-22 40025511 300.1 ns/op 304 B/op 3 allocs/op BenchmarkUniqueTagsPractical/prealloc-22 38560572 308.3 ns/op 304 B/op 3 allocs/op BenchmarkUniqueTagsPractical/array-search-22 56932317 210.9 ns/op 304 B/op 3 allocs/op
tiedotguy
commented
Jul 2, 2025
| b.Run(name, func(b *testing.B) { | ||
| b.ReportAllocs() | ||
| for b.Loop() { | ||
| _ = f(slices.Clone(t1), slices.Clone(t2)) |
Collaborator
Author
There was a problem hiding this comment.
Technically we don't need to clone t2, but I had the code written...
hstan
approved these changes
Jul 4, 2025
hstan
requested changes
Jul 4, 2025
Member
hstan
left a comment
hstan
left a comment
There was a problem hiding this comment.
It seems I discovered a bug when trying to run a unit test to understand how the uniqueTagsSimple works:
with input:
{
name: "multiple duplicates",
t1: gostatsd.Tags{"a", "b", "a", "c", "b"},
t2: gostatsd.Tags{"a", "b", "c"},
expected: gostatsd.Tags{"a", "b", "c"},
},
the output of uniqueTagsSimple(t1, t2) is gostatsd.Tags{"a", "b", "b", "c"} instead of gostatsd.Tags{"a", "b", "c"}
hstan
reviewed
Jul 4, 2025
|
|
||
| last := len(t1) | ||
| for idx := 1; idx < last; { // start at 1 because we know the first item will be unique. | ||
| if slices.Contains(t1[:idx-1], t1[idx]) { |
Member
There was a problem hiding this comment.
should here be t1[:idx] to reach the last element? since the loop condition is idx < len(t1)?
(see my other comment for a failed test case)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We have two general code paths:
We used to handle number 1 by passing an empty map to number 2, however these can be seen as fast (add static tags) and slow (add static tags + perform filtering) code-paths.
The fast code path is now optimized by performing an array search instead of using a map to track things that have been seen. This is more efficient for small values. There is diminishing returns eventually, but that's with a high number of tags, which is an anti-pattern.
The primary benefit is in removing the map allocation.
The slow code path is unchanged, but could be improved by ensuring the map is sufficiently large, or using the same linear search method to check if a tag should be excluded.
Note: this benchmark isn't perfect because we clone the source arrays, but that can be identified in the profile.